-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AG-NONE Cleanup destroy #1571
AG-NONE Cleanup destroy #1571
Conversation
0901081
to
684fbff
Compare
300ae3d
to
e6f0f10
Compare
(this.annotationManager = new AnnotationManager(chart.annotationRoot)), // NOSONAR | ||
(this.ariaAnnouncementService = new AriaAnnouncementService(this.scene.canvas.element)), // NOSONAR | ||
(this.chartEventManager = new ChartEventManager()), // NOSONAR | ||
(this.contextMenuRegistry = new ContextMenuRegistry()), // NOSONAR | ||
(this.cursorManager = new CursorManager(this.domManager)), // NOSONAR | ||
(this.highlightManager = new HighlightManager()), // NOSONAR | ||
(this.interactionManager = new InteractionManager(chart.keyboard, this.domManager)), // NOSONAR | ||
(this.keyNavManager = new KeyNavManager(this.interactionManager, this.domManager)), // NOSONAR | ||
(this.focusIndicator = new FocusIndicator(this.domManager)), // NOSONAR | ||
(this.regionManager = new RegionManager(this.interactionManager, this.keyNavManager, this.focusIndicator)), // NOSONAR | ||
(this.toolbarManager = new ToolbarManager()), // NOSONAR | ||
(this.gestureDetector = new GestureDetector(this.domManager)), // NOSONAR | ||
(this.layoutService = new LayoutService()), // NOSONAR | ||
(this.updateService = new UpdateService(updateCallback)), // NOSONAR | ||
(this.proxyInteractionService = new ProxyInteractionService(this.updateService, this.focusIndicator)), // NOSONAR | ||
(this.seriesStateManager = new SeriesStateManager()), // NOSONAR | ||
(this.callbackCache = new CallbackCache()), // NOSONAR | ||
(this.animationManager = this.createAnimationManager(this.interactionManager, updateMutex)), // NOSONAR | ||
(this.dataService = new DataService<any>(this.animationManager)), // NOSONAR | ||
(this.tooltipManager = new TooltipManager(this.domManager, chart.tooltip)) // NOSONAR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These inline assignment and returns are not very readable, can you adjust the pattern here to avoid this and the need for // NOSONAR
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is readable.
Sonar argues that assignments in expressions are discouraged is because they can have an unexpected side-effect. For example:
if (isEnabled && x = getValue()) {
doSomething();
}
In this scenario, it is not clear what the programmer meant:
- Assign x to a value if
isEnabled
istrue
? - Do something if
isEnabled
is true and x is assigned to a true-y value? - Or did the programmer mean
==
/===
instead of=
?
That can be confusing, because x
might be assigned or it might not, and doSomething()
might be called or it might not.
However, this argument is not applicable in this case. How can (this.annotationManager = new AnnotationManager(chart.annotationRoot)), // NOSONAR
possibly be misunderstood? There no other logical operators like &&
and ||
so there is only 1 possible outcome from this expression.
Also, it is abundantly clear the intent is to assign a value and we didn't intend ==
or ===
(because this expression is part of ChartContext
's constructor and also why would we want to compare an unassigned value to a newly constructed object?).
The aim objective of ObjectDestroyer
is to follow a RAII model to ensure that:
- We do not pass uninitialised variables to the constructors
- We run the destructors in reverse
The only option to avoid the // NOSONAR
could be to use lambdas methods that ObjectDestroyer
can call:
this.owned = new ObjectDestroyer(
() => this.annotationManager = new AnnotationManager(chart.annotationRoot),
// ...
);
However, this will violate objective 1). We could end up accidentally creating a circular dependency amongst constructors.
We could also apply the changes that Sonar suggests, which involve putting the assignments as separate expressions
this.annotationManager = new AnnotationManager(chart.annotationRoot);
this.ariaAnnouncementService = new AriaAnnouncementService(this.scene.canvas.element);
this.chartEventManager = new ChartEventManager();
// ...
this.owned = new ObjectDestroyer(
this.domManager,
this.annotationManager,
this.ariaAnnouncementService,
this.chartEventManager,
// ..
);
However, this is not a good suggestion because objective 2) is to ensure that destructors are run in reverse. But with this approach we need to manually ensure the ordering is reversed, which is basically what we're doing now. So we might as well not bother adding ObjectDestroyer
if we're going with this approach.
It is a pain to have to write // NOSONAR
in each line. I would much prefer multi-line enable/disable options but I can't find something like that. Open to other suggestions to disable the Sonar warnings, but Sonar is wrong in this case.
@@ -0,0 +1,14 @@ | |||
type Destroyable = {} | { destroy(): void }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why allow objects without a destroy()
- I think that will lead to more cases where a missing destroy()
is not flagged up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK. Some of the objects in ChartContext
do not have a destructors, but we can add empty destroy()
methods to these.
7c97dcb
to
9c86fc5
Compare
Discussed offline. We've decided to ditch the |
No description provided.